Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make serve catalog use decorator #784

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jwlodek
Copy link
Member

@jwlodek jwlodek commented Sep 6, 2024

Checklist

  • Add a Changelog entry
  • Add the ticket number which this PR closes to the comment section - N/A

@danielballan
Copy link
Member

Thanks @jwlodek! I am curious what led you to this change.

I had to grep around to reconstruct my reasoning here. When I added this command we already had:

tiled serve config ...
tiled serve pyobject ...
tiled serve directory ...

and we were adding:

tiled catalog init
tiled catalog upgrade-database
tiled catalog downgrade-database

I couldn't decide which of these two was more obvious:

tiled serve catalog
tiled catalog serve

It seemed inevitable that people would misremember it 50% of the time and feel annoyed, so I registered the same function for both, as you can see in this grep output.

$ git grep serve_catalog
tiled/_tests/test_cli.py:def test_serve_catalog_temp(args, tmp_path):
tiled/commandline/_catalog.py:from ._serve import serve_catalog
tiled/commandline/_catalog.py:catalog_app.command("serve")(serve_catalog)
tiled/commandline/_serve.py:def serve_catalog(
tiled/commandline/_serve.py:serve_app.command("catalog")(serve_catalog)

Should we add a comment to the code explaining this?

@jwlodek
Copy link
Member Author

jwlodek commented Sep 7, 2024

As we discussed a few weeks ago, I'd like to put together a CLI utility for standing up a tiled server for dev work with all the same handlers etc. as beamline tiled, and since I've started prototyping this I had a look at how the tiled cli works currently.

I hadn't seen the same registration in catalog, so I thought this was just something that was missed when moving from explicit registration to decorators for the other functions. I think the reasoning for registering both this way is sound, it makes sense to me. I think a comment explaining the reasoning is a good idea too, so when someone else has a look through the code they don't also assume it was just something that was missed. I'll modify this PR to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants